[rtl] Fix counter reset value on FPGA #2226
Conversation
rtl/ibex_counter.sv
Outdated
| @@ -55,8 +55,12 @@ module ibex_counter #( | |||
| localparam int DspPragma = CounterWidth < 49 ? "yes" : "no"; | |||
| (* use_dsp = DspPragma *) logic [CounterWidth-1:0] counter_q; | |||
|
|
|||
| // DSP output register requires synchronous reset. | |||
| `define COUNTER_FLOP_RST posedge clk_i | |||
| if (CounterWidth < 49) begin : g_sync_reset | |||
There was a problem hiding this comment.
Could you explain the magic number "49". Also line 55 declares DspPragma as an int but initializes with strings. Maybe put both the counter_q and the `define inside the if-generate?
Also, could you please move the `undef COUNTER_FLOP_RST line right past its last (and only) use at line 71?
There was a problem hiding this comment.
It's an implementation detail for Xilinx FPGAs. A single DSP48 primitive can readily do up to a 48-bit counter.
There was a problem hiding this comment.
Not an expert in this - why do we need this pragma anyways? Wouldn't the synthesis tool detect this counter and try using a DSP?
There was a problem hiding this comment.
I don't know. I assume someone tried it and wasn't getting the result they wanted, so they added the synthesis attribute.
That's the impression I get from the PR where it originated: #624
There was a problem hiding this comment.
Could you explain the magic number "49". Also line 55 declares DspPragma as an int but initializes with strings. Maybe put both the counter_q and the `define inside the if-generate?
Also, could you please move the `undef COUNTER_FLOP_RST line right past its last (and only) use at line 71?
Done, thanks for the feedback!
I don't know. I assume someone tried it and wasn't getting the result they wanted, so they added the synthesis attribute.
That's the impression I get from the PR where it originated: #624
Ah thanks for linking this PR.
There was a problem hiding this comment.
Ok to merge this now? It will unblock some SiVal tasks.
There was a problem hiding this comment.
Yes, let's merge this. CI ran through already.
nasahlpa
force-pushed
the
fix_counter_reset_value
branch
from
a8416cb to
105bb12
Compare
If the counter width is >= 49, we do not use a DSP on the FPGA. Then, we should use an asynchronous reset to initialize the counter. This bug was detected when enabling the lockstep for the CW340. A lockstep mismatch happend as the mcycle counters of the main and shadow core did not match due to this bug. Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa
force-pushed
the
fix_counter_reset_value
branch
from
105bb12 to
3428e0c
Compare
There was a problem hiding this comment.
Excellent work @nasahlpa , this looks good to me!
Yes, indeed the FPGA synthesis tool needs to be explicitly told to use DPS slices here and for counters wider than 48 bits, it will use 2 DSP slices and quite some logic to combine them. From a logic utilization and timing perspective this is not nice. Back in 2020 Noam (back then a student at ETH) was investigating how and where the code can be better mapped to FPGA. IIRC, the biggest improvements where achieved in the register file (using LUTRAM instead of FFs) and the performance counters.
If the counter width is >= 49, we do not use a DSP on the FPGA. Then, we should use an asynchronous reset to initialize the counter.
This bug was detected when enabling the lockstep for the CW340. A lockstep mismatch happend as the mcycle counters of the main and shadow core did not match due to this bug.